Add AMP-first preview and args to validate request for Site Scanning #6615

westonruter · 2021-09-18T04:29:01Z

Summary

Validate Request Args

The amp_validate query var has been improved so that instead of taking a single string value for the nonce, it instead can be an array of args, including:

nonce (string): What will be checked against AMP_Validation_Manager::get_amp_validate_nonce()
cache (bool): Whether the validation results will be stored in an amp_validated_url post.
cached_if_fresh (bool): Whether previously-stored results will be returned if there is a non-stale amp_validated_url post for the current URL.
omit_stylesheets (bool): Whether to omit the stylesheets key from the response.

For example, attempting to get the getting results for the homepage can look like a request to the following URL:

https://wordpressdev.lndo.site/?amp_validate[nonce]=e9ca7665d29fab856e78a6ff23da8979&amp_validate[omit_stylesheets]=true&amp_validate[cached_if_fresh]=true&amp_validate[cache]=true

When such a cached request is made, the response will include a new key:

"validated_url_post": {
    "id": 1,
    "edit_link": "https://wordpressdev.lndo.site/core-dev/src/wp-admin/post.php?post=3778&action=edit"
}

The edit link here is the Validated URL screen where the results can be viewed. See #6610 (comment). Partially addresses #4979.

A revalidated prop is also included which indicates whether the previously-cached data was returned (if false) or whether the page was re-validated (true).

AMP-first Override

The AMP plugin's default template mode is Reader Nevertheless, when doing Site Scanning we want to check the site as if Standard template mode is active, so we can check the active theme for AMP compatibility issues. In order to facilitate this, a new amp-first query parameter is introduced which allows an authenticated user or a validation request force Standard mode. The logic employed for this override is adopted back from the Web Stories plugin first introduced in GoogleForCreators/web-stories-wp#3621. The conditions are a bit convoluted, but much of the admin-specific logic will be able to be eliminated once we move away from classic admin screens in favor of doing more RESTful requests for validation data. See #4719 (comment).

Checklist

My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

includes/validation/class-amp-validated-url-post-type.php

…ilable" This reverts commit 808d969.

…ate-store-url-query-var * 'develop' of github.com:ampproject/amp-wp: (67 commits) Ignore `nested-interactive` Axe rule Set appropriate node and npm versions in package.json Downgrade package-lock.json back to v1 Improve test coverage Add special case logic for gutenberg_inject_default_block_context Improve method ordering Remove needless check for filter-specific add_block_source_comments Run wrap_block_callbacks at PHP_INT_MIN Update handle_block_source_comment_replacement to handle wrappers added in wrap_block_callbacks Simplify logic in wrap_block_callbacks Bump svgo from 2.6.0 to 2.6.1 Advise to check loopback request test Simplify language for condition for what headers are looked for Bump eslint-plugin-jest from 24.4.0 to 24.4.2 Improve test coverage for get_persistent_object_cache_learn_more_action Remove absolete wp_doing_ajax filter Eliminate use of deprecated assertion Store results of page caching to reduce severity of missing a persistent object cache Explain how page cache detection is performed Improve tests for page caching ...

github-actions · 2021-09-27T19:25:54Z

Plugin builds for c226596 are ready 🛎️!

Download development build
Download production build

westonruter · 2021-09-28T17:05:19Z

src/QueryVar.php

+	 * @see \AMP_Validation_Manager::has_cap()
+	 * @var string
+	 */
+	const AMP_FIRST = 'amp-first';


Note that the hyphenated amp-first is used because it's intended to be user-facing.

~~The new amp_store query parameter is not user-facing, and goes along with amp_validate. So that's why I chose an underscore.~~

westonruter · 2021-09-30T16:18:32Z

Also add a parameter to allow returning cached results if they exist.
Indicate if the results are stale (via \AMP_Validated_URL_Post_Type::get_post_staleness()) when passing amp_store. Include as meta information under amp_validated_url.
The \AmpProject\AmpWP\Validation\ScannableURLProvider::get_urls() method should return results based on the currently-selected template mode. If using legacy reader theme, only the singular post or page should be returned as URLs. (Or rather, they can just skip since AMP is not available.)
The Settings screen (and WP Cron) should run a scan based on the selected template mode, whereas the Onboarding WIzard should force AMP-first.

westonruter · 2021-10-06T04:51:38Z

@delawski One more thing I expect will be useful for you:

Allow omission of stylesheet data from response via a query parameter.

Since the stylesheet data is by far the most massive part of the validation response, excluding it should make the screens load faster.

delawski · 2021-10-06T09:36:22Z

Allow omission of stylesheet data from response via a query parameter.

It sounds really good! Stylesheet data is huge and we don't really need it for Site Scan.

delawski · 2021-10-06T16:45:35Z

The \AmpProject\AmpWP\Validation\ScannableURLProvider::get_urls() method should return results based on the currently-selected template mode. If using legacy reader theme, only the singular post or page should be returned as URLs. (Or rather, they can just skip since AMP is not available.)

One thing to note here is that we are fetching the scannable URLs at the beginning of the Onboarding Wizard flow. They are used later in the Site Scan. I guess we should be able to override the template mode when retrieving the URLs so that we always scan against the Standard mode.

… stylesheets

westonruter · 2021-10-07T05:21:41Z

One thing to note here is that we are fetching the scannable URLs at the beginning of the Onboarding Wizard flow. They are used later in the Site Scan. I guess we should be able to override the template mode when retrieving the URLs so that we always scan against the Standard mode.

In regards to the URLs returned by ScannableURLProvider::get_urls() needing to include all templates even when legacy Reader mode is selected: this seems to be the case. The reason is that ScannableURLProvider is not being constructed with any URLScanningContext, so this is resulting in all templates being returned. So at the moment you should be in good shape to use the existing endpoint you have.

westonruter · 2021-10-07T15:28:42Z

@delawski I suppose after this PR we could eliminate the URLValidationRESTController service entirely since it is redundant while also being more limited since it only allows for validation of posts and also slower in that it requires a loopback request. Naturally this change could be done in a subsequent PR, assuming the changes in this PR check out.

includes/validation/class-amp-validation-manager.php

dhaval-parekh · 2021-10-25T19:00:44Z

includes/validation/class-amp-validation-manager.php

+	 *
+	 * @param array      $sanitization_results Sanitization results.
+	 * @param int        $status_code          Status code.
+	 * @param array|null $last_error           Last error.


Just a thought, Instead of taking $last_error as an argument, can we assign it in the function itself? Since, it is return value of error_get_last();

We could, but I thought this may be preferable since it is explicit input and less reliance on global state, so it is easier to test.

dhaval-parekh

Changes look good to me.

fellyph · 2021-12-10T11:42:55Z

QA Passed

cache
cache_bust
nonce
omit_stylesheets

Requests

Allow validate requests to also store results

30301da

westonruter added this to the v2.2 milestone Sep 18, 2021

westonruter mentioned this pull request Sep 18, 2021

Add Site Scan Feature #6610

Merged

6 tasks

delawski reviewed Sep 22, 2021

View reviewed changes

includes/validation/class-amp-validated-url-post-type.php Outdated Show resolved Hide resolved

westonruter added 10 commits September 23, 2021 10:40

Make is_validate_request a public method and member var protected

6f34359

fixup! Allow validate requests to also store results

f57e397

Use constant for amp_store query param

af3c45b

Remove needless pretty-printing of validation response

f930949

Add amp_available_override query param to force AMP to be available

808d969

WIP: Try facilitating selective AMP-first URLs when in paired mode

c2a4738

Adopt the Web Stories approach of selectively enabling Standard mode

ee5cd4b

Revert "Add amp_available_override query param to force AMP to be ava…

3eadf15

…ilable" This reverts commit 808d969.

Move validate logic to validation manager

8a61a74

westonruter changed the title ~~Allow validate requests to also store results~~ Add AMP-first preview for Site Scanning and allow validate requests to also store results Sep 27, 2021

westonruter added the Site Scanning label Sep 27, 2021

westonruter added 2 commits September 27, 2021 11:20

Improve send_validate_response logic and add test coverage

b3a12bd

Add test coverage for AMP-first override requests

c24e0ef

westonruter marked this pull request as ready for review September 27, 2021 19:20

westonruter requested a review from pierlon September 27, 2021 19:20

westonruter mentioned this pull request Sep 27, 2021

Wizard Step: Plugin compatibility scanning #4719

Closed

westonruter commented Sep 28, 2021

View reviewed changes

westonruter added 3 commits October 6, 2021 16:33

Introduce args for validate requests to cache, serve cached, and omit…

06284df

… stylesheets

Move logic into maybe_send_cached_validate_response method

aa02b96

Replace redundante staleness with revalidated prop

4d12ffe

westonruter added 3 commits October 6, 2021 20:36

Test omission of stylesheets in send_validate_response

c3294c9

Add test for maybe_send_cached_validate_response

cdc0132

Improve tests for send_validate_response

9f3a80c

westonruter requested a review from delawski October 7, 2021 04:50

westonruter changed the title ~~Add AMP-first preview for Site Scanning and allow validate requests to also store results~~ Add AMP-first preview and args to validate request for Site Scanning Oct 7, 2021

westonruter mentioned this pull request Oct 8, 2021

REST endpoints for AMP validation errors and validated URLs #5552

Closed

5 tasks

delawski force-pushed the add/validate-store-url-query-var branch from 9f3a80c to 5b46ffd Compare October 13, 2021 13:34

westonruter requested review from dhaval-parekh and milindmore22 October 22, 2021 17:51

delawski force-pushed the add/validate-store-url-query-var branch from 5b46ffd to 5fc2692 Compare October 25, 2021 11:33

westonruter force-pushed the add/validate-store-url-query-var branch from 5fc2692 to 9f3a80c Compare October 25, 2021 18:35

westonruter commented Oct 25, 2021

View reviewed changes

includes/validation/class-amp-validation-manager.php Outdated Show resolved Hide resolved

dhaval-parekh reviewed Oct 25, 2021

View reviewed changes

dhaval-parekh approved these changes Oct 25, 2021

View reviewed changes

Remove extraneous action priority

c226596

westonruter merged commit 746d7ac into develop Oct 26, 2021

westonruter deleted the add/validate-store-url-query-var branch October 26, 2021 00:02

delawski mentioned this pull request Nov 3, 2021

Force Standard mode when fetching scannable URLs in Wizard #6683

Merged

2 tasks

fellyph self-assigned this Dec 9, 2021

fellyph removed their assignment Dec 10, 2021

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add AMP-first preview and args to validate request for Site Scanning #6615

Add AMP-first preview and args to validate request for Site Scanning #6615

Uh oh!

westonruter commented Sep 18, 2021 •

edited

Loading

Uh oh!

Uh oh!

github-actions bot commented Sep 27, 2021 •

edited

Loading

Uh oh!

westonruter Sep 28, 2021 •

edited

Loading

Uh oh!

westonruter commented Sep 30, 2021 •

edited

Loading

Uh oh!

westonruter commented Oct 6, 2021 •

edited

Loading

Uh oh!

delawski commented Oct 6, 2021

Uh oh!

delawski commented Oct 6, 2021

Uh oh!

westonruter commented Oct 7, 2021

Uh oh!

westonruter commented Oct 7, 2021 •

edited

Loading

Uh oh!

Uh oh!

dhaval-parekh Oct 25, 2021

Uh oh!

westonruter Oct 25, 2021

Uh oh!

dhaval-parekh left a comment

Uh oh!

fellyph commented Dec 10, 2021

Uh oh!

Uh oh!

Add AMP-first preview and args to validate request for Site Scanning #6615

Add AMP-first preview and args to validate request for Site Scanning #6615

Uh oh!

Conversation

westonruter commented Sep 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Validate Request Args

AMP-first Override

Checklist

Uh oh!

Uh oh!

github-actions bot commented Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonruter Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter commented Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonruter commented Oct 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

delawski commented Oct 6, 2021

Uh oh!

delawski commented Oct 6, 2021

Uh oh!

westonruter commented Oct 7, 2021

Uh oh!

westonruter commented Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dhaval-parekh Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

westonruter Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

dhaval-parekh left a comment

Choose a reason for hiding this comment

Uh oh!

fellyph commented Dec 10, 2021

Uh oh!

Uh oh!

westonruter commented Sep 18, 2021 •

edited

Loading

github-actions bot commented Sep 27, 2021 •

edited

Loading

westonruter Sep 28, 2021 •

edited

Loading

westonruter commented Sep 30, 2021 •

edited

Loading

westonruter commented Oct 6, 2021 •

edited

Loading

westonruter commented Oct 7, 2021 •

edited

Loading